Profile photo not refreshing after upload in Client Profile #2519
Conversation
trim.1DD19614-3186-4A87-B516-55F18FA6BC55.MOV |
| when (event) { | ||
| ClientProfileEditEvent.NavigateBack -> onNavigateBack() | ||
| ClientProfileEditEvent.OnSaveSuccess -> { | ||
| onNavigateBack() |
There was a problem hiding this comment.
I commented it because it is not needed for the navigation, and it is excess code
The screen navigation works well with or without it
There was a problem hiding this comment.
Remove it if not needed
| MifosBasicDialog( | ||
| title = "Success", | ||
| content = { Text("Your profile image has been updated successfully!") }, | ||
| confirmText = "Done", |
There was a problem hiding this comment.
use Success and Failure Dilaogs instead of Basic Dialog.
There was a problem hiding this comment.
This one
MifosStatusDialog
which accepts success and Failure state and also move strings to string resources
| <string name="feature_client_profile_update_success_message">Your profile image has been updated successfully!</string> | ||
| <string name="feature_client_profile_update_error_message">We couldn’t update your profile image. Please try again.</string> | ||
| <string name="should_refresh">should_refresh</string> |
There was a problem hiding this comment.
This should be added to feature:client's commonMain strings.xml (android-client\feature\client\src\commonMain\composeResources\values\strings.xml)
There was a problem hiding this comment.
Oh okay, I will do that and push it
WalkthroughThis PR introduces a status dialog UI component with supporting string resources, implements profile refresh signaling via saved state across screens, updates the client profile editing flow to use status dialogs instead of event-based navigation, and adds logic to fetch and display updated profile images after successful uploads. Changes
Sequence DiagramsequenceDiagram
participant Screen as ClientProfileEditScreen
participant VM as ClientProfileEditViewModel
participant Repo as Repository
participant BackStack as SavedStateHandle
Screen->>VM: Upload profile image
VM->>Repo: uploadImage(image)
Repo-->>VM: Success
VM->>Repo: fetchUpdatedImage()
Repo-->>VM: Image data
VM->>VM: Update profileImage state
VM->>VM: Set DialogState to Success
VM-->>Screen: StateFlow emits Success
Screen->>Screen: Show MifosStatusDialog(SUCCESS)
Screen->>Screen: User taps Continue
Screen->>BackStack: Set PROFILE_SHOULD_REFRESH_KEY = true
Screen->>Screen: Navigate away / Dismiss
participant ProfileScreen
ProfileScreen->>BackStack: Read PROFILE_SHOULD_REFRESH_KEY
BackStack-->>ProfileScreen: true
ProfileScreen->>VM: Trigger OnRetry action
VM->>Repo: Fetch profile data
ProfileScreen->>BackStack: Set PROFILE_SHOULD_REFRESH_KEY = false
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
cmp-android/src/main/res/values/strings.xml (1)
426-426: This string resource appears unrelated to the PR objective and was previously flagged for relocation.The
numberofrepaymentsstring does not relate to profile photo refresh functionality. Additionally, a previous review comment indicated this should be added tofeature:client's commonMain strings.xml (android-client\feature\client\src\commonMain\composeResources\values\strings.xml) instead.
🧹 Nitpick comments (2)
cmp-android/src/main/kotlin/cmp/android/app/ComponentActivityExtensions.kt (1)
84-85: Remove redundantdistinctUntilChanged()call.The
distinctUntilChanged()operator is applied to the flow at line 84, but the same operator is also applied inside thecombineblock at line 51. This results in duplicate filtering.Consider removing one of the
distinctUntilChanged()calls to simplify the flow chain:} - .distinctUntilChanged() .conflate()feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientDetailsProfile/ClientProfileDetailsScreen.kt (1)
22-22: Remove unused import.The
profile_should_refreshstring resource is imported but never used in this file. The code uses the constantPROFILE_SHOULD_REFRESH_KEYfrom the utils package instead.-import androidclient.feature.client.generated.resources.profile_should_refresh
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
cmp-android/build.gradle.kts(2 hunks)cmp-android/src/main/kotlin/cmp/android/app/ComponentActivityExtensions.kt(3 hunks)cmp-android/src/main/res/values/strings.xml(1 hunks)core/designsystem/src/commonMain/composeResources/values/strings.xml(1 hunks)core/designsystem/src/commonMain/kotlin/com/mifos/core/designsystem/component/MifosBasicDialog.kt(3 hunks)core/ui/src/commonMain/kotlin/com/mifos/core/ui/components/MifosBreadCrumb.kt(3 hunks)core/ui/src/commonMain/kotlin/com/mifos/core/ui/util/Constants.kt(1 hunks)feature/client/src/commonMain/composeResources/values/strings.xml(1 hunks)feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientDetailsProfile/ClientProfileDetailsScreen.kt(4 hunks)feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientEditProfile/ClientProfileEditScreen.kt(3 hunks)feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientEditProfile/ClientProfileEditViewModel.kt(5 hunks)feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientProfile/ClientProfileScreen.kt(3 hunks)feature/client/src/commonMain/kotlin/com/mifos/feature/client/utils/Constants.kt(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientEditProfile/ClientProfileEditScreen.kt (1)
core/designsystem/src/commonMain/kotlin/com/mifos/core/designsystem/component/MifosBasicDialog.kt (1)
MifosStatusDialog(217-278)
🔇 Additional comments (6)
cmp-android/build.gradle.kts (1)
10-10: Formatting improvements align with Kotlin/Android conventions.The addition of a blank line after the license header (line 10) and the newline at end-of-file (line 158) follow standard Kotlin style practices and improve visual separation between the header block and code logic.
Also applies to: 158-158
core/ui/src/commonMain/kotlin/com/mifos/core/ui/components/MifosBreadCrumb.kt (2)
85-88: LGTM! Profile refresh signal correctly propagated on breadcrumb navigation.The implementation correctly sets the refresh flag in the previous back stack entry's saved state before navigating, ensuring the target screen can detect and respond to profile updates.
101-106: LGTM! Refresh signal properly set on back navigation.The back button handler correctly propagates the refresh signal to the previous screen before popping the back stack.
cmp-android/src/main/kotlin/cmp/android/app/ComponentActivityExtensions.kt (1)
56-62: LGTM! Formatting improved for readability.The multi-line formatting of the
SystemBarStyle.autocall improves code readability without changing behavior.feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientDetailsProfile/ClientProfileDetailsScreen.kt (1)
95-108: LGTM! Profile refresh mechanism implemented correctly.The implementation properly:
- Observes the refresh flag from saved state
- Triggers a retry action when the flag is set
- Resets the flag after handling
- Provides a fallback for null savedStateHandle
This enables the screen to refresh profile details when navigating back after an update.
feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientProfile/ClientProfileScreen.kt (1)
67-80: LGTM! Consistent profile refresh implementation.The refresh mechanism is correctly implemented and consistent with the pattern used in ClientProfileDetailsScreen. The implementation properly observes the saved state flag, triggers a retry, and resets the flag after handling.
| <string name="core_designsystem_dialog_success">Success</string> | ||
| <string name="core_designsystem_dialog_failure">Failure</string> | ||
| <string name="core_designsystem_dialog_continue">Continue</string> | ||
| <string name="core_designsystem_mifosStatusDialog">MifosStatusDialog</string> |
There was a problem hiding this comment.
Keep dialog test tag string non-translatable.
Line 22 declares core_designsystem_mifosStatusDialog, which feeds directly into a Modifier.testTag. If translators localize this value, every UI test that targets that tag will fail. Please mark it non-translatable (or inline a constant) so the tag stays stable across locales.
- <string name="core_designsystem_mifosStatusDialog">MifosStatusDialog</string>
+ <string name="core_designsystem_mifosStatusDialog" translatable="false">MifosStatusDialog</string>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <string name="core_designsystem_mifosStatusDialog">MifosStatusDialog</string> | |
| <string name="core_designsystem_mifosStatusDialog" translatable="false">MifosStatusDialog</string> |
🤖 Prompt for AI Agents
In core/designsystem/src/commonMain/composeResources/values/strings.xml around
line 22, the string resource core_designsystem_mifosStatusDialog is used as a
testTag and must not be translated; change its declaration to mark it
non-translatable (e.g., add translatable="false" to the <string> tag) or replace
usages with an inline constant so the test tag value remains stable across
locales.
| @@ -0,0 +1,3 @@ | |||
| package com.mifos.core.ui.util | |||
|
|
|||
| internal const val PROFILE_SHOULD_REFRESH_KEY = "profile_should_refresh" No newline at end of file | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Consolidate duplicate constant definition.
The same constant PROFILE_SHOULD_REFRESH_KEY with identical value is defined in both:
core/ui/src/commonMain/kotlin/com/mifos/core/ui/util/Constants.kt(this file)feature/client/src/commonMain/kotlin/com/mifos/feature/client/utils/Constants.kt
Since both are marked internal, they cannot be shared across modules. Consider making the core/ui version public and removing the duplicate from feature/client to maintain a single source of truth.
Apply this approach:
- Make this constant public (or internal with module export if using Kotlin Multiplatform module structure)
- Remove the duplicate from feature/client/utils/Constants.kt
- Update imports in feature/client files to use the core/ui constant
🤖 Prompt for AI Agents
In core/ui/src/commonMain/kotlin/com/mifos/core/ui/util/Constants.kt around line
3, the constant PROFILE_SHOULD_REFRESH_KEY is declared internal and duplicated
in feature/client; make this the single source of truth by changing its
visibility to public (or configure module export for KMP if you prefer keeping
it internal across modules), remove the duplicate declaration from
feature/client/src/commonMain/kotlin/com/mifos/feature/client/utils/Constants.kt,
and update all imports/usages in feature/client to reference
com.mifos.core.ui.util.PROFILE_SHOULD_REFRESH_KEY from the core/ui module.
| MifosStatusDialog( | ||
| status = MifosDialogStatus.FAILURE, | ||
| message = stringResource(Res.string.profile_update_error_message), | ||
| onDismissRequest = { onAction(ClientProfileEditAction.DismissModalBottomSheet) } | ||
| ) | ||
| } | ||
|
|
||
| is ClientProfileEditState.DialogState.Success -> { | ||
| MifosStatusDialog( | ||
| status = MifosDialogStatus.SUCCESS, | ||
| message = stringResource(Res.string.update_success_message), | ||
| onDismissRequest = { onAction(ClientProfileEditAction.DismissModalBottomSheet) } | ||
| ) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Verify navigation occurs after successful profile update.
The switch to MifosStatusDialog for error and success states improves UX consistency. However, the previous EventsEffect handling for OnSaveSuccess navigation was removed. Please verify that dismissing the success dialog triggers the appropriate navigation action (e.g., DismissModalBottomSheet should navigate back or trigger the refresh mechanism).
🏁 Script executed:
#!/bin/bash
# Search for the ViewModel implementation to verify success state handling
rg -A 10 "ClientProfileEditAction.DismissModalBottomSheet" --type=kotlin
# Check if DialogState.Success is properly set in the ViewModel
rg -B 5 -A 5 "DialogState.Success" --type=kotlin -g '*ClientProfileEdit*'Length of output: 8200
I need to verify if the navigation logic is still present after dismissing the success dialog.
I need to verify if there's a LaunchedEffect or similar mechanism that triggers navigation when the dialog state is cleared.
🏁 Script executed:
#!/bin/bash
# Search for LaunchedEffect and navigation logic in the screen
rg -A 20 "LaunchedEffect\|navController\|navigate\(" --type=kotlin -g '*ClientProfileEditScreen*'
# Also check the complete dialog state handling
rg -B 10 "when.*dialogState" --type=kotlin -g '*ClientProfileEditScreen*'Length of output: 1463
🏁 Script executed:
#!/bin/bash
# Get the full ClientProfileEditScreen composable function signature and context
rg -B 5 "fun ClientProfileEditScreen" --type=kotlin -A 30 -g '*ClientProfileEditScreen*'
# Search for EventsEffect pattern mentioned in review comment
rg -B 3 -A 10 "EventsEffect\|OnSaveSuccess" --type=kotlin -g '*ClientProfileEdit*'Length of output: 5004
🏁 Script executed:
#!/bin/bash
# Search for navController usage in ClientProfileEditScaffold
rg -A 100 "fun ClientProfileEditScaffold" --type=kotlin -g '*ClientProfileEditScreen*' | head -120
# Check if onNavigateBack is called or if there's any side effect after dialog state change
rg "onNavigateBack\|LaunchedEffect.*dialogState" --type=kotlin -g '*ClientProfileEditScreen*'Length of output: 15376
Add navigation trigger after successful profile update.
The success dialog now dismisses without navigating. When DismissModalBottomSheet clears the dialog state, there's no mechanism to trigger navigation back. Add a LaunchedEffect in ClientProfileEditScreen to observe state.dialogState and call onNavigateBack() after a successful update completion, or handle navigation in the ViewModel by triggering a navigation action when DismissModalBottomSheet follows a successful profile update.
🤖 Prompt for AI Agents
In
feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientEditProfile/ClientProfileEditScreen.kt
around lines 199-212, the success dialog currently only dismisses without
navigating back; add a LaunchedEffect that observes state.dialogState and when
it transitions to a success dialog then is dismissed, call onNavigateBack() (or
alternatively emit a navigation action from the ViewModel when
DismissModalBottomSheet follows a success) so the UI navigates back after a
successful profile update; ensure the LaunchedEffect only triggers once per
success transition to avoid duplicate navigations.
| clientDetailsRepo.getImage(route.id).collect { result -> | ||
| when (result) { | ||
| is DataState.Success -> mutableStateFlow.update { | ||
| it.copy( | ||
| profileImage = imageToByteArray(result.data), | ||
| dialogState = ClientProfileEditState.DialogState.Success, | ||
| ) | ||
| } | ||
| is DataState.Loading -> mutableStateFlow.update { | ||
| it.copy(dialogState = ClientProfileEditState.DialogState.Loading) | ||
| } | ||
| else -> Unit | ||
| } | ||
| } |
There was a problem hiding this comment.
Surface an error when the refresh fetch fails.
Inside the success branch we collect clientDetailsRepo.getImage, but if that refresh comes back as DataState.Error we never update the state—dialogState stays at Loading, so the user is stuck watching a spinner even though the upload succeeded. Please handle the error emission and surface it through DialogState.Error.
when (result) {
is DataState.Success -> mutableStateFlow.update {
it.copy(
profileImage = imageToByteArray(result.data),
dialogState = ClientProfileEditState.DialogState.Success,
)
}
is DataState.Loading -> mutableStateFlow.update {
it.copy(dialogState = ClientProfileEditState.DialogState.Loading)
}
+ is DataState.Error -> mutableStateFlow.update {
+ it.copy(
+ dialogState = ClientProfileEditState.DialogState.Error(
+ result.message ?: "Unknown Error",
+ ),
+ )
+ }
else -> Unit📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| clientDetailsRepo.getImage(route.id).collect { result -> | |
| when (result) { | |
| is DataState.Success -> mutableStateFlow.update { | |
| it.copy( | |
| profileImage = imageToByteArray(result.data), | |
| dialogState = ClientProfileEditState.DialogState.Success, | |
| ) | |
| } | |
| is DataState.Loading -> mutableStateFlow.update { | |
| it.copy(dialogState = ClientProfileEditState.DialogState.Loading) | |
| } | |
| else -> Unit | |
| } | |
| } | |
| clientDetailsRepo.getImage(route.id).collect { result -> | |
| when (result) { | |
| is DataState.Success -> mutableStateFlow.update { | |
| it.copy( | |
| profileImage = imageToByteArray(result.data), | |
| dialogState = ClientProfileEditState.DialogState.Success, | |
| ) | |
| } | |
| is DataState.Loading -> mutableStateFlow.update { | |
| it.copy(dialogState = ClientProfileEditState.DialogState.Loading) | |
| } | |
| is DataState.Error -> mutableStateFlow.update { | |
| it.copy( | |
| dialogState = ClientProfileEditState.DialogState.Error( | |
| result.message ?: "Unknown Error", | |
| ), | |
| ) | |
| } | |
| else -> Unit | |
| } | |
| } |
🤖 Prompt for AI Agents
In
feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientEditProfile/ClientProfileEditViewModel.kt
around lines 138 to 151, the collect on clientDetailsRepo.getImage only handles
Success and Loading so a DataState.Error leaves dialogState stuck at Loading;
update the collector to handle DataState.Error by calling
mutableStateFlow.update to set dialogState =
ClientProfileEditState.DialogState.Error (include the error message/exception
from the DataState.Error payload if available) and clear Loading so the UI shows
an error state instead of a spinner.
| @@ -0,0 +1,3 @@ | |||
| package com.mifos.feature.client.utils | |||
|
|
|||
| internal const val PROFILE_SHOULD_REFRESH_KEY = "profile_should_refresh" No newline at end of file | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove duplicate constant definition.
This constant duplicates the one defined in core/ui/src/commonMain/kotlin/com/mifos/core/ui/util/Constants.kt. Please remove this duplicate and use the core/ui constant instead to maintain a single source of truth.
🤖 Prompt for AI Agents
In
feature/client/src/commonMain/kotlin/com/mifos/feature/client/utils/Constants.kt
around line 3, there is a duplicate constant PROFILE_SHOULD_REFRESH_KEY; remove
this duplicate definition and update any local references in the feature/client
module to use the single constant from core/ui
(com.mifos.core.ui.util.Constants.PROFILE_SHOULD_REFRESH_KEY) by adding the
appropriate import where needed; ensure you delete the line defining the
constant here, remove any now-unused imports, and run a build to confirm all
references resolve to the core constant.
3 participants
Fixes - Jira-#Issue_Number
Didn't create a Jira ticket, click here to create new.
Please Add Screenshots If there are any UI changes.
Please make sure these boxes are checked before submitting your pull request - thanks!
Run the static analysis check
./gradlew checkorci-prepush.shto make sure you didn't break anythingIf you have multiple commits please combine them into one commit by squashing them.
Summary by CodeRabbit
New Features
Chores